-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ ADD scheme package for AddToScheme #770
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #770 +/- ##
==========================================
+ Coverage 67.16% 67.21% +0.04%
==========================================
Files 22 23 +1
Lines 1465 1467 +2
==========================================
+ Hits 984 986 +2
Misses 415 415
Partials 66 66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I had in mind, thanks for looking into it.
It looks like imports need to be sorted and we should be good to go.
I didn't catch a few lines here not sure exactly how they are being used.
I think it is dead code. We can probably just remove it.
Hi @m1kola , I remove those few lines but I saw some linting errors from a tool called gci but I couldn't determine what version you use to lint the project. Could point me to the right github repo? Then I can run it and do a clean up in order to merge the code. Thanks! |
Never mind actually. I figured it out. Bingo! It's fixed now. I believe we're good to go. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks you!
Never mind actually. I figured it out. Bingo!
Great! Take a look at make help
- our Makefile
has some useful commands for development. Like make run
will spin up a Kind cluster with all the required components installed + will compile operator-controller from local source code and deploy it as well.
If you have any questions about development process - don't hesitate to ask on issues/PR or on Kubernetes slack (channel #olm-dev
).
Trying to merge again. I think I introduced a flake in #785 and that's why e2e failed in merge queue. |
712cd04
Thanks @m1kola ! |
Description
This PR adds a scheme package providing a central location for new apis to be added to scheme.
Also replaces the use of a new scheme instance and var by importing the package scheme and using scheme.Scheme.
Solves #322
Reviewer Checklist